Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dml: support default expression cache when insert #15216

Merged
merged 18 commits into from
Mar 19, 2020

Conversation

AilinKid
Copy link
Contributor

@AilinKid AilinKid commented Mar 9, 2020

What problem does this PR solve?

support default expression cache when insert.
It will accelerate the eval speed of the column with a default expression.
simple bench:

create sequence seq;
create table t(a int default next value for seq)
insert into t values(),(),(),()... *1000

before

insert consume: 50.498672ms

now

insert consume: 33.213615ms

What is changed and how it works?

Check List

Tests

  • Unit test
  • Integration test

Release note

  • dml: support default expression cache when insert.

@AilinKid AilinKid requested a review from a team as a code owner March 9, 2020 05:20
@ghost ghost requested review from SunRunAway and XuHuaiyu and removed request for a team March 9, 2020 05:20
@AilinKid AilinKid changed the title dmldefault expr cache dml: support default expression cache when insert Mar 9, 2020
@github-actions github-actions bot added sig/sql-infra SIG: SQL Infra sig/execution SIG execution labels Mar 9, 2020
@codecov
Copy link

codecov bot commented Mar 9, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@2b47a08). Click here to learn what that means.
The diff coverage is 77.7777%.

@@             Coverage Diff             @@
##             master     #15216   +/-   ##
===========================================
  Coverage          ?   80.3768%           
===========================================
  Files             ?        502           
  Lines             ?     133857           
  Branches          ?          0           
===========================================
  Hits              ?     107590           
  Misses            ?      17827           
  Partials          ?       8440

val types.Datum
// default expr like `nextval(seq)` should be evaluated row by row.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How are other default expressions implemented? e.g. current_timestamp().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

insert into (now()),(now())... only eval once, treated as constant.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment format is like expr(variable name) ....

for i := 0; i < 100; i++ {
s.tk.MustExec(sql)
}
// fmt.Println(time.Now().Sub(start) / 100)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

surely, this just show how we bench it.

@ghost ghost removed their request for review March 18, 2020 10:05
@AilinKid
Copy link
Contributor Author

/build

Copy link
Contributor

@crazycs520 crazycs520 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@AilinKid AilinKid added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Mar 18, 2020
@AilinKid
Copy link
Contributor Author

/run-all-tests

djshow832
djshow832 previously approved these changes Mar 18, 2020
@AilinKid
Copy link
Contributor Author

/merge

@sre-bot sre-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 18, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Mar 18, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Mar 18, 2020

@AilinKid merge failed.

@AilinKid
Copy link
Contributor Author

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Mar 19, 2020

/run-all-tests

@AilinKid
Copy link
Contributor Author

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Mar 19, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Mar 19, 2020

@AilinKid merge failed.

@AilinKid
Copy link
Contributor Author

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Mar 19, 2020

/run-all-tests

@sre-bot sre-bot merged commit 3ced6bc into pingcap:master Mar 19, 2020
@you06 you06 added this to the v4.0.0-rc milestone Mar 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/execution SIG execution sig/sql-infra SIG: SQL Infra status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants